-
-
Notifications
You must be signed in to change notification settings - Fork 555
fix: chrome should not wrap mid-line #1423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
partially reverts commit "e011cf4863e06b6eb07985f379f7750d76b091ee"
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This reverts commit 12e8a31.
partially reverts commit "e011cf4863e06b6eb07985f379f7750d76b091ee"
I went back on this approach again. I found that the pseudo element can use a border so that it does not take space within the content, but still have a width to allow mouse events to work on the element. Seems to be working well in firefox & chrome in the various scenarios we have identified. Will want to look into this in the morning though.... |
@@ -77,9 +77,7 @@ export const createCollaborationExtensions = (collaboration: { | |||
labelElement.setAttribute("style", `background-color: ${user.color}`); | |||
labelElement.insertBefore(document.createTextNode(user.name), null); | |||
|
|||
cursorElement.insertBefore(document.createTextNode("\u2060"), null); // Non-breaking space |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now let CSS insert these
Screen.Recording.2025-02-11.at.09.20.27.mp4Left Firefox, Middle Chrome, Right Safari I think I've got all of the edge cases figured out for the major browsers here. They each needed their own hacks. I went with the most chrome native implementation (using the pseudo element with a border) and then added the hacks for firefox & safari behavior on top with comments on why. Hopefully this will resolve it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome. Worth the effort 🙌
- There's a little bit of whitespace now again right?
- shall we comment the scenarios as to why this was complicated? (I hope we're not touching this code for a while, but maybe good to explain that changes need to be tested on all browsers and both the long lines + whitespace end of line scenarios?)
/* Add nbsp; to each side of the caret */ | ||
.collaboration-cursor__caret::after, | ||
.collaboration-cursor__caret::before { | ||
content: ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which one is here? Github doesn't show it
nbsp = https://www.compart.com/en/unicode/U+00A0
word joiner = https://www.compart.com/en/unicode/U+2060
the previous code had "word joiner"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
word joiner is there. I'll update the comment with the unicode to make it clear
There is but the negative margin slurps it so it becomes effectively 0px wide (though Safari seems to not agree with that unless position: relative is set).
Yea, I'll add a here be dragons comment |
Partially reverts commit "e011cf4863e06b6eb07985f379f7750d76b091ee"
This addresses a bug introduced by #1422 which caused a regression where chrome in some instances would break text wrapping because of the pseudo element's presence in the document.
The pseduo element is now removed, moving back to the original border color approach with an additional fix specifically applied to firefox that will cause the element not to wrap when whitespace is following text